refactor: Extract cni manager as the public part #1719
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1719 +/- ##
==========================================
- Coverage 58.68% 58.56% -0.13%
==========================================
Files 200 200
Lines 15565 15514 -51
==========================================
- Hits 9135 9085 -50
- Misses 5143 5145 +2
+ Partials 1287 1284 -3
|
cri/ocicni/cni_manager.go
Outdated
| // Teardown network if an error returned. | ||
| err := c.plugin.TearDownPod(*podNetwork) | ||
| if err != nil { | ||
| logrus.Errorf("failed to detroy network for sandbox %q: %v", podNetwork.ID, err) |
cri/ocicni/types.go
Outdated
| @@ -0,0 +1,24 @@ | |||
| package ocicni | |||
There was a problem hiding this comment.
I do not think defining an interface should use a filename of types.go.
Instead, maybe cnimanager.go is a better one. WDYT?
There was a problem hiding this comment.
cnimanager.go doesn't make sense, there is a file named cni_manager.go. So maybe apis.go or something else? WDYT?
There was a problem hiding this comment.
@YaoZengzeng
Any advice?
There was a problem hiding this comment.
I think types.go is fine, we could expand it if we need more types 😄
|
Sounds good to me, but some nits still need update. @starnop |
cri/ocicni/cni_manager.go
Outdated
|
|
||
| import ( | ||
| "fmt" | ||
| "github.com/alibaba/pouch/cri/config" |
There was a problem hiding this comment.
please move it into the group.
cri/v1alpha1/cri_network.go
Outdated
| func (c *CniManager) Status() error { | ||
| return c.plugin.Status() | ||
| } | ||
|
|
||
| // toCNIPortMappings converts CRI port mappings to CNI. | ||
| func toCNIPortMappings(criPortMappings []*runtime.PortMapping) []ocicni.PortMapping { |
There was a problem hiding this comment.
Maybe move this function to cri_utils.go and rm this file.
There was a problem hiding this comment.
Well, maybe we'll consider creating separate files in the future when we have more utility functions.
cri/v1alpha2/cri_network.go
Outdated
| func (c *CniManager) Status() error { | ||
| return c.plugin.Status() | ||
| } | ||
|
|
||
| // toCNIPortMappings converts CRI port mappings to CNI. | ||
| func toCNIPortMappings(criPortMappings []*runtime.PortMapping) []ocicni.PortMapping { |
Signed-off-by: Starnop <[email protected]>
|
PTAL @YaoZengzeng |
|
LGTM |
Signed-off-by: Starnop [email protected]
Ⅰ. Describe what this PR did
For now，The organization of the code about CNI Manager is unreasonable，we should reorganize the file structure: extract cni manager as the public part.
Ⅱ. Does this pull request fix one issue?
fixes part of #1524
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews